-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
Fix 7180 autodetect #7691
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix 7180 autodetect #7691
Conversation
Are you ok with updating the docs as follows? @jorisvandenbossche @jreback
|
is |
I think it used to be different. Actually I have to check whether the current implementation respects that. You think it's confusing to have |
ok it appears that both
|
This was meant to fix a regression, where width auto-detection no longer works. Why constantly introduce unnecessary changes to existing behavior? there's no reason. xref the discussion in #7750 |
@armaganthis3 I guess you are not reading my comments. I asked @bjonen to confirm what the functionaility is. I don't really recall what its supposed to be. Of course 0 and None should, all other things being equal, have the same meaning. Unless their is REALLY REALLY good reason. Why don't you do some research and post it here. That would actually be helpful. |
You asked him to add a test to ensure they behave the same, and you did not read the docstring
Utter nonsense. You are making stuff up and then proclaiming them as rules. It's a minor point, |
Ok, both before and in the current PR, |
@bjonen Your updated description in the comment above looks OK. Can you update the PR for this? A question: what means 'autodetect' in the case of rows? |
Sure I can do that. Autodetect for rows:
There are two parameters:
|
Thanks for the clarification! |
8003764
to
2537c4d
Compare
Added docs and rebased. Ready from my side. |
looks fine, @jorisvandenbossche ? |
@bjonen actually, can you add a release note for this in v0.15.0. Is a bigger notice needed? (its sort fo an edge case, thinking no) |
for a dataframe prints out fully or just a summary repr. | ||
'None' value means unlimited. | ||
If max_rows is exceeded, switch to truncate view. View | ||
display.max_columns for details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would leave 'real' explanation of what max_rows
does also here, instead of referring to max_cols (I think it is annoying if you do pd.describe_option('max_rows)
and you don't get the answer).
@bjonen I added still some comments on the docstring (sorry for not doing this before you updated and rebased) I think this is a bug fix? (and so should go in that section?) |
Something else, I tested it, and it doesn't seem to work correctly for me (tested it on Windows) On the standard size (width of 80), it gives one column too much (and overflows). If I resize the terminal, then it works (eg with width of 75).
And
|
@jorisvandenbossche Thanks for looking at this PR.
|
If you feel the speed issue is a deal breaker, I can attempt an alternative implementation. But this will take some time. If you want to get this into the next release, I could take care of the overflow issue and then mention that this display gets slow for dfs greater than (100,100). |
@bjonen you can check up front whether the size is an issue (e.g. > 100x100)? |
3fec539
to
7d027e0
Compare
@jreback Following your suggestion the speed improves quite a bit. Without: And with auto detection: I fixed the overflow issue on my machine. It would be great if someone could check it (especially on windows). Also updated the docs and rebased. Thanks for the help guys! |
this is only for max_columns=o right? (eg auto detection is on) |
Yes, continuing the same session: In [7]: pd.options.display.max_rows = 20 |
However, just found these two cases are broken: max_cols == 0 and max_rows != 0 |
7d027e0
to
1f67279
Compare
Fixed remaining issues and added line in release note. |
Checked in the notebook and the truncation seems unaffected by the changes. @jreback @jorisvandenbossche |
@bjonen what do you mean with "have to set width==0"? |
Sorry, that was not very clear. I meant However, I checked briefly and It shouldn't be necessary to adjust this value to get the df displayed across the entire terminal when max_columns = 0. |
@bjonen I can confirm that it now works as expected on Windows It is also quicker, although you notice it is a bit slower than normal (it is not 'irritating' anymore, but if it would be easily possible to make it as fast as the normal display, that would be a plus I think, but maybe not for this PR) |
But I noticed something else in the printing, not necessarily related to this (it occurs both in max_columns=0 case as default parameter).
Update: opened a seperate issue about this, also occurs on master. |
@jorisvandenbossche Thanks for testing the PR! I'm glad to hear it's working on Windows as well. |
Regarding the speed issue: Right now, I don't see a quick fix. So I would do this in a separate PR. |
looks fine to me.just needs a rebase. This is simply a bug fix, so looks ok, unless @jorisvandenbossche thinks should be more prominent in release notes. |
no, looks ok |
This PR closes #7180
In the terminal:
If maxcols == 0 then auto detect columns
if maxrows == 0 auto detect rows